-
Notifications
You must be signed in to change notification settings - Fork 347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(alternative proposal) update how callback plugin gets copied and added to job container #1093
(alternative proposal) update how callback plugin gets copied and added to job container #1093
Conversation
ansible_runner/config/_base.py
Outdated
@@ -267,6 +268,14 @@ def _prepare_env(self, runner_mode='pexpect'): | |||
if callback_dir is None: | |||
callback_dir = get_callback_dir() | |||
self.env['ANSIBLE_CALLBACK_PLUGINS'] = ':'.join(filter(None, (self.env.get('ANSIBLE_CALLBACK_PLUGINS'), callback_dir))) | |||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not
followed by an else
is kinda breaking my brain. How about we just flip these conditions? if self.containerized ... else
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amended
a08300a
to
8174803
Compare
# when containerized, copy the callback dir to $pirvate_data_dir/artifacts/<job_id>/callback | ||
# than append to env['Ansible_CALLBACK_PLUGINS'] with the copyied location | ||
callback_dir = os.path.join(self.artifact_dir, 'callback') | ||
shutil.copytree(get_callback_dir(), callback_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this and saw:
drwxrwxrwx. 3 alancoding alancoding 4096 May 2 15:53 callback
These permissions are going to set off alarm bells. We probably need to pre-create the directory so we can set specific permissions. Practically, we'd probably copy from above in this same file
ansible-runner/ansible_runner/config/_base.py
Line 138 in 8174803
os.makedirs(self.artifact_dir, exist_ok=True, mode=0o700) |
but os.mkdir
might be preferable here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed with @AlanCoding about the topic, the copytree inherited the file permission of the parent ansible-runner installation... so long as the permission for the original install is good the copied version would be good as well
Great work! You knocked out the tests really fast. I ran the demo project, and wanted to paste this for illustration.
This requires: diff --git a/demo/env/settings b/demo/env/settings
index 693b1d3..32e7b82 100644
--- a/demo/env/settings
+++ b/demo/env/settings
@@ -2,3 +2,5 @@
idle_timeout: 600
job_timeout: 3600
pexpect_timeout: 10
+process_isolation: true
+process_isolation_executable: podman This has an aesthetic consequence for users, and we might need to consider adding a blurb in the docs to give them confidence that this is intended. If we can accept this added folder, then I believe this is a more maintainable pattern moving forward. It makes sense to me that we would cut a copy for every run (every job For testing - I think we may need to consider the possibility that someone re-runs the same private_data_dir & ident. If we try to re-copy, I'm unclear what will happen. To spitball an expectation - I would prefer that we delete the existing folder. |
6c52498
to
33d09be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-wise, this looks complete at the moment. We might want some docs changes, since this touches the artifacts folder. I would consider back-porting to release_2.2, because I anticipate others having the same problems you had.
ansible_runner/config/_base.py
Outdated
@@ -262,7 +263,18 @@ def _prepare_env(self, runner_mode='pexpect'): | |||
self.fact_cache = os.path.join(self.artifact_dir, self.settings['fact_cache']) | |||
|
|||
# Use local callback directory | |||
if not self.containerized: | |||
if self.containerized: | |||
# when containerized, copy the callback dir to $pirvate_data_dir/artifacts/<job_id>/callback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# when containerized, copy the callback dir to $pirvate_data_dir/artifacts/<job_id>/callback | |
# when containerized, copy the callback dir to $private_data_dir/artifacts/<job_id>/callback |
ansible_runner/config/_base.py
Outdated
if not self.containerized: | ||
if self.containerized: | ||
# when containerized, copy the callback dir to $pirvate_data_dir/artifacts/<job_id>/callback | ||
# than append to env['Ansible_CALLBACK_PLUGINS'] with the copyied location. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# than append to env['Ansible_CALLBACK_PLUGINS'] with the copyied location. | |
# than append to env['ANSIBLE_CALLBACK_PLUGINS'] with the copied location. |
same here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me except for the typos @Akasurde mentioned. thank you for your work :)
33d09be
to
a2503f5
Compare
commit amended with fix to type |
ansible_runner/config/_base.py
Outdated
if not self.containerized: | ||
if self.containerized: | ||
# when containerized, copy the callback dir to $private_data_dir/artifacts/<job_id>/callback | ||
# than append to env['ANSIBLE_CALLBACK_PLUGINS'] with the copyied location. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# than append to env['ANSIBLE_CALLBACK_PLUGINS'] with the copyied location. | |
# then append to env['ANSIBLE_CALLBACK_PLUGINS'] with the copied location. |
ansible_runner/config/_base.py
Outdated
# when containerized, copy the callback dir to $private_data_dir/artifacts/<job_id>/callback | ||
# than append to env['ANSIBLE_CALLBACK_PLUGINS'] with the copyied location. | ||
callback_dir = os.path.join(self.artifact_dir, 'callback') | ||
# if callback dir already exist (on repeat execution with same ident), remove it first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# if callback dir already exist (on repeat execution with same ident), remove it first. | |
# if callback dir already exists (on repeat execution with the same ident), remove it first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only two typos left :) Code looks great, thank you!
I gotta go find me a good vscode plugin >.< |
when in containerized environment - always copy callback plugin to private_data_dir/artifacts/{job_id}/callback - appending to `ANSIBLE_CALLBACK_PLUGINS` with the runner callback plugin location (from private_data/artifacts) - unused method `utils.callback_mount` - update unit tests Signed-off-by: Hao Liu <haoli@redhat.com> Co-Authored-By: Alan Rominger <arominge@redhat.com> Signed-off-by: Hao Liu <haoli@redhat.com> Signed-off-by: Hao Liu <the.real.hao.liu@gmail.com>
a2503f5
to
d73fe9f
Compare
commit amended with fix to grammatical error |
no worries .... I do that all the time :D Luckily shrews still puts up with me |
…sible#1093) when in containerized environment - always copy callback plugin to private_data_dir/artifacts/{job_id}/callback - appending to `ANSIBLE_CALLBACK_PLUGINS` with the runner callback plugin location (from private_data/artifacts) - unused method `utils.callback_mount` - update unit tests Signed-off-by: Hao Liu <haoli@redhat.com> Co-Authored-By: Alan Rominger <arominge@redhat.com> Signed-off-by: Hao Liu <haoli@redhat.com> Signed-off-by: Hao Liu <the.real.hao.liu@gmail.com> Co-authored-by: Alan Rominger <arominge@redhat.com> (cherry picked from commit 3b74385)
…sible#1093) when in containerized environment - always copy callback plugin to private_data_dir/artifacts/{job_id}/callback - appending to `ANSIBLE_CALLBACK_PLUGINS` with the runner callback plugin location (from private_data/artifacts) - unused method `utils.callback_mount` - update unit tests Co-authored-by: Alan Rominger <arominge@redhat.com> (cherry picked from commit 3b74385) Signed-off-by: Hao Liu <the.real.hao.liu@gmail.com>
…sible#1093) when in containerized environment - always copy callback plugin to private_data_dir/artifacts/{job_id}/callback - appending to `ANSIBLE_CALLBACK_PLUGINS` with the runner callback plugin location (from private_data/artifacts) - unused method `utils.callback_mount` - update unit tests Co-authored-by: Alan Rominger <arominge@redhat.com> (cherry picked from commit 3b74385) Signed-off-by: Hao Liu <the.real.hao.liu@gmail.com>
…sible#1093) when in containerized environment - always copy callback plugin to private_data_dir/artifacts/{job_id}/callback - appending to `ANSIBLE_CALLBACK_PLUGINS` with the runner callback plugin location (from private_data/artifacts) - unused method `utils.callback_mount` - update unit tests Co-authored-by: Alan Rominger <arominge@redhat.com> (cherry picked from commit 3b74385) Signed-off-by: Hao Liu <the.real.hao.liu@gmail.com>
) (#1098) when in containerized environment - always copy callback plugin to private_data_dir/artifacts/{job_id}/callback - appending to `ANSIBLE_CALLBACK_PLUGINS` with the runner callback plugin location (from private_data/artifacts) - unused method `utils.callback_mount` - update unit tests Co-authored-by: Alan Rominger <arominge@redhat.com> (cherry picked from commit 3b74385) Signed-off-by: Hao Liu <the.real.hao.liu@gmail.com> Co-authored-by: Alan Rominger <arominge@redhat.com>
alternative proposal for issue #1088
when in containerized environment
ANSIBLE_CALLBACK_PLUGINS
with the runner callback plugin location (from private_data/artifacts)Co-Authored-By: Alan Rominger arominge@redhat.com
Signed-off-by: Hao Liu haoli@redhat.com